-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add SpEL support for nested username extraction in OAuth2 user info #16857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR @yybmion! I wonder if this might be better implemented using SpEL to provide more powerful options for resolving the username. What are your thoughts? |
Hi @rwinch , Thank you for your guidance on this. I initially chose the dot notation approach because it offers a simple and intuitive solution specifically for the nested user-name-attribute issue. However, I can see the value in using SpEL as you suggested. While I think it may be slightly more complex, SpEL provides much greater extensibility for future use cases beyond simple nested structures. The consistency with other parts of the Spring Security framework is also a advantage. If you confirm that SpEL is the preferred direction, I'd be happy to update the PR accordingly. |
Yes. Please provide an implementation that uses SpEL. |
Hello @rwinch, I'd like to clarify your feedback on my PR about supporting nested properties in the user-name-attribute. Did you mean that I should implement support for expressions like Thank you for your guidance! |
I think an outline would be: Allow Injecting the Principal Name into
|
This comment was marked as resolved.
This comment was marked as resolved.
c5aa6d9
to
972afda
Compare
Hi @rwinch thank you for the detailed guide. I've implemented the SpEL-based approach as outlined in the original issue. The solution provides
Please see the PR description for detailed changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates. This is taking shape. I've provided feedback inline.
.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java
Outdated
Show resolved
Hide resolved
.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java
Outdated
Show resolved
Hide resolved
.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java
Outdated
Show resolved
Hide resolved
return this.usernameExpression; | ||
} | ||
|
||
return this.userNameAttributeName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just return the usernameExpression
without defaulting here. Instead ensure that the userNameAttributeName
setter sets the usernameExpression
.
@@ -672,7 +700,14 @@ private ProviderDetails createProviderDetails(ClientRegistration clientRegistrat | |||
providerDetails.tokenUri = this.tokenUri; | |||
providerDetails.userInfoEndpoint.uri = this.userInfoUri; | |||
providerDetails.userInfoEndpoint.authenticationMethod = this.userInfoAuthenticationMethod; | |||
if (this.usernameExpression != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you update the builder to set the expression when setting the userNameAttributeName
, then the if statement can be removed
return new DefaultOAuth2User(authorities, attributes, userNameAttributeName); | ||
|
||
String evaluatedUsername = evaluateUsername(attributes, usernameExpression); | ||
Collection<GrantedAuthority> authorities = getAuthorities(token, attributes, usernameExpression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now passing in usernameExpression
to getAuthorities
which means that the usernameExpression
is now passed into OAuth2UserAuthority
in place of userNameAttributeName
.
I think when users use more complex expressions it will break functionality introduced gh-15012 because the OAuth2UserAuthority
only has an attribute name and not a property. We will need to figure out how to integrate cleanly with gh-15012 From the PR it sounds like we could just expose the username rather than the attribute, but I'm not certain how the integration is being used cc @filiphr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for highlighting this important integration issue with gh-15012!
I've addressed this by following your suggestion to expose the username directly rather than the attribute name.
- Added
username
field to OAuth2UserAuthority withgetUsername()
method - Builder pattern:
OAuth2UserAuthority.withUsername(evaluatedUsername)
- Pass the evaluated username result (not the SpEL expression) to OAuth2UserAuthority
I think this way, OAuth2UserAuthority always receives the final evaluated username string, preserving compatibility with gh-15012 regardless of expression complexity.
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Show resolved
Hide resolved
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Show resolved
Hide resolved
...uth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java
Outdated
Show resolved
Hide resolved
- Add username field to DefaultOAuth2User for direct name injection - Add Builder pattern with DefaultOAuth2User.withUsername(String) static factory method - Deprecate constructor that uses nameAttributeKey lookup in favor of Builder pattern - Update Jackson mixins to support username field serialization/deserialization This change prepares for SpEL support in the next commit. Signed-off-by: yybmion <yunyubin54@gmail.com>
0f6d3ce
to
5798e87
Compare
Thanks for the feedback @rwinch ! I've addressed all the review comments and updated the code accordingly. But the build failure appears to be related to Kotlin version. What would you recommend I do? |
- Add usernameExpression property with SpEL evaluation support - Auto-convert userNameAttributeName to SpEL for backward compatibility - Use SimpleEvaluationContext for secure expression evaluation - Pass evaluated username to OAuth2UserAuthority for spring-projectsgh-15012 compatibility - Add Builder pattern to DefaultOAuth2User - Support nested property access (e.g., "data.username") Fixes spring-projectsgh-16390 Signed-off-by: yybmion <yunyubin54@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting very close!
Please address the comments and ensure to update the documentation (including the What's New Section)
@@ -44,6 +45,8 @@ public class OAuth2UserAuthority implements GrantedAuthority { | |||
|
|||
private final String userNameAttributeName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property should be deprecated as should all the methods/constructors that use it.
Please also address OidcUserAuthority
which extends from this class. It will likely need it's own Builder
that extends from this Builder
and have an overridden withUsername
static method.
* the attributes - preserved for backwards compatibility | ||
* @param username the username | ||
*/ | ||
private OAuth2UserAuthority(String authority, Map<String, Object> attributes, String userNameAttributeName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the Builder doesn't use usernameAttributeName
, we shouldn't include this as a property here, so that this constructor does not deprecated properties.
* the attributes - preserved for backwards compatibility | ||
* @param username the username | ||
*/ | ||
private OAuth2UserAuthority(String authority, Map<String, Object> attributes, String userNameAttributeName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the order of username, authority, attributes since username is now a required property.
* This method provides direct access to the username without requiring knowledge of | ||
* the attribute structure or SpEL expressions used to extract it. | ||
* @return the username | ||
* @since 6.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR targets main, so this (and any other since) should be since 7.0
This PR adds support for extracting usernames from nested properties in OAuth2 user info responses using SpEL expressions, addressing the limitation where providers wrap user data in nested objects.
Fixes #16390
Problem
OAuth2 providers (like X/Twitter) wrap user data in nested objects, requiring complex workarounds to extract usernames.
Solution
Commit 1: Allow injecting principal name into DefaultOAuth2User
Commit 2: Add SpEL support for nested username extraction
OAuth2UserAuthority
#15012 compatibilityBackward Compatibility